Add multiview limits and tests #8206
Conversation
inner-daemons
requested review from
cwfitzgerald
and removed request for
cwfitzgerald
|
@cwfitzgerald I left a comment about this in matrix but it got hidden by newer messages. Is it a bad idea to add another multi view related field to a render pass descriptor? This might be a topic worth bringing up in a meeting, I'm not sure. I'm just hesitant to assume that's fine because if I'm wrong then I will have refactored every use of render passes in this entire repo :) Not urgent of course EDIT: This has been answered |
…ue to bitmask limitation
…amples multiview fields
|
This PR alone, which should be rather simple, has killed ALL of my confidence in myself with using Metal lmao. I genuinely cannot figure out what is going wrong. @cwfitzgerald Can you get copilot to try to "review" this again |
|
It be like that sometimes! Do you have access to a Mac? |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 96 out of 103 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Yep, I've been trying this on a Mac the whole time. XCode debugging and all. (its completely useless from what I can tell, it didn't catch what was obviously undefined behavior and it never provided any useful output). |
|
Wow, the future of AI is here! |
wgpu-hal/src/vulkan/adapter.rs
Outdated
| /// Pulled out of my ass | ||
| const MULTIVIEW_INSTANCE_MINIMUM: u32 = 256 * 1024 * 1024; |
There was a problem hiding this comment.
This is one of my concerns here. Vulkan exposes a limit to the instance index, which technically could be anything to my knowledge. All backends would normally have an intrinsic limit of u32::MAX for such calls (except maybe metal, unimportant). This limit happens to be ~1/16 of u32::MAX which should be fine on most platforms. I'm not aware of any situation where you would issue a draw call over 1 billion instances to a realtime VR headset. Unfortunately, this is still unsound.
Currently, for standard/indexed draw calls we check against this and panic in hal if its violated.
There was a problem hiding this comment.
checking the vulkan spec in table 70 lists the minimum for maxMultiviewInstanceIndex being 2²⁷-1 (or 134217727)
would it help at all to use that number instead?
There was a problem hiding this comment.
checking the vulkan spec in table 70 lists the minimum for
maxMultiviewInstanceIndexbeing2²⁷-1(or134217727) would it help at all to use that number instead?
Thanks for looking at this! Knowing that's vulkan's lower limit, its probably fine to just have no lower bound in wgpu itself.
|
@cwfitzgerald A seemingly unrelated CI fail happened here. It only occured once, pushing an empty commit made it go away. Is this worth looking into? |
|
Yeah we've had that intermittently falling. Could you file an issue? |
|
Welp, its a very strange issue that I discovered. But apparently you must Also, you only need to set 1 viewport, but you can still give higher viewport indices apparently? I am very confused |
4 participants
Connections
Addresses #7196 and #8138
Description
Adds new limits and testing for multiview, add support for more backends (also in naga), add new multiview bitmask renderpass descriptor field for selecting which layers to render to in a single pass.
Marking as open because its had progress made and I think this is about what it will look like when complete, even though its completely broken at the moment. Also because I want to get it in the pipeline sooner rather than later. Just the following remaining.
TODO:
Testing
There are tests
Squash or Rebase?
Squash
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.